fix(cli): sample real pixels behind hidden text for contrast-audit backgrounds#2102
Conversation
…round detection SVG text (<text>/<tspan>/<textPath>) is painted via the `fill` property, not `color` — the two are independent in SVG. When a composition sets `fill` without also setting `color`, getComputedStyle(el).color resolves to the inherited/initial value (often black), which does not match what's actually rendered on screen. The contrast audit was reading `cs.color` unconditionally, so any SVG text with fill != color got a fabricated foreground and a false pass/fail verdict. Add an SVG-aware foreground read: for elements inside an <svg> (detected via ownerSVGElement), prefer the computed `fill` when it resolves to a solid rgb()/rgba() color, falling back to `color` for paint values that aren't a plain color (none, context-fill, gradient/pattern refs) so we never crash parseColor or report a fabricated black. Mirrored the same fix in skills/hyperframes-creative/scripts/contrast-report.mjs, which duplicates the DOM-walk/foreground-read logic (not just the WCAG math), and updated the header sync note (the referenced path had also drifted to skills/hyperframes-creative). Extracted the pure decision into contrast-fg.ts (mirroring the existing contrast-bg.ts pattern) with unit tests, since the browser-injected script itself can't be unit-tested without a real page. Manually verified with a standalone puppeteer-core repro against the cached chrome-headless-shell binary: an SVG <text fill="white"> with no `color` set on a black background. Before: fg=rgb(0,0,0) bg=rgb(0,0,0) ratio=1 wcagAA=false (false fail) After: fg=rgb(255,255,255) bg=rgb(0,0,0) ratio=21 wcagAA=true (correct) Also verified fill: none and fill: url(#gradient) fall back to `color` without crashing. Scope: this addresses only the SVG fill-vs-color mismatch. The other 4 reported contrast-audit false-positive patterns (cross-comp color bleed, solid-fill button/pill, translucent glass-text layers, translucent decorative/animated elements) are out of scope and remain open.
…t backgrounds
The contrast audit estimated an element's background by sampling a 4px
ring just outside its bounding box. That proximity heuristic is wrong
whenever what's immediately outside the text differs from what's
actually behind it:
- cross-component bleed: text sits near the edge of its own panel/
layer, and a differently-colored sibling panel/layer starts just
outside the text's bbox, so the ring samples the neighbor instead
of the real background.
- backdrop-filter glass text: a translucent blurred panel sized only
a couple pixels larger than the text — the ring exits the panel and
samples the raw, unblurred, untinted pixels behind it.
- partially-overlapping translucent decoration: a decorative shape
that only partly overlaps the ring, or sits entirely INSIDE the
text's own bbox, which the ring can never see regardless of size.
(Solid-fill pill/button backgrounds were already handled correctly by
the existing own-background ancestor walk and are unaffected by this
change — confirmed via repro, not touched.)
Fix: hide each candidate text element's own paint (color/fill set to
transparent, layout-neutral — no reflow), take ONE screenshot with the
glyphs invisible, then sample the real composited pixels directly
INSIDE the element's own bbox. No proximity heuristic needed since
we're reading the exact pixels that were behind the glyphs. Same
number of screenshots as before (one per sample time), just moved to
after the hide instead of before it.
This changes contrast-audit.browser.js from a single __contrastAudit
call into a two-phase __contrastAuditPrepare / __contrastAuditFinish
contract (see validate.ts's runContrastAudit for the calling side, with
a try/finally restore-safety-net so a mid-loop failure can't leave
later samples auditing a page with stale hidden text). Mirrored the
same change in skills/hyperframes-creative/scripts/contrast-report.mjs,
which duplicates the same DOM-walk/sampling logic — there the visible
frame still comes from the producer's normal capture path (for the
overlay image); only the background-sampling capture is a plain
page.screenshot() after hiding text, deliberately bypassing the
capture pipeline's static-frame dedup cache, which knows nothing about
the DOM mutation and would hand back a stale pre-mutation buffer.
Added contrast-sample.ts (mirroring the existing contrast-bg.ts /
contrast-fg.ts pattern) hosting the pure sample-rect/grid-point
computation, unit tested, since the two browser-injected scripts can't
import it directly.
Verified via a standalone puppeteer-core harness against real fixtures
for each pattern, then end-to-end through the actual `hyperframes
validate --contrast` CLI command against a real scaffolded project:
before this change, a text run sitting mostly on a translucent
decorative badge reported as a false PASS (ring never sees decoration
fully inside the bbox); after, it correctly reports ~1.1:1 and fails.
A cross-component-bleed case and a backdrop-filter glass-text case that
previously reported false FAILs (ring bleeding into a neighboring
panel / the raw unblurred backdrop) now correctly report as passing,
matching their true rendered contrast.
miguel-heygen
changed the title
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at e0ff3b7.
Solid piece of work — the ring → inside-bbox shift is a real architectural improvement, the SVG fill fix is orthogonal but justified, and the two-phase prepare / finish contract is cleanly done. Both commit messages are exemplary root-cause writeups. Inline comments below on three concerns (all around the "hidden text" safety net's edges), plus a note on the 5th-pattern verification. None gating.
The 5th (solid-fill pill) "investigated, didn't reproduce" claim is credible on its own merits — the new inside-bbox sampling handles rounded pills through pill-color-dominates-median, and the historical pickOpaqueBackground ancestor-walk is preserved for legacy callers. Worth noting there's no in-repo regression test for the pill class: contrast-sample.test.ts covers the pure rect/grid math, contrast-fg.test.ts covers foreground resolution, but neither pins the rounded-pill scenario end-to-end. If it ever regresses, CI wouldn't catch it — only a manual repro would. A rounded-pill DOM fixture added to layout-audit.browser.test.ts (which already sets up contrast-audit.browser.js via the two-phase entry) would be cheap.
CI is green at HEAD on the new run (29031178042), with Windows render + Tests still in progress at review time.
| var sample = function (sx, sy) { | ||
| if (sx < 0 || sx >= w || sy < 0 || sy >= h) return; | ||
| var idx = (sy * w + sx) * 4; | ||
| window.__contrastAuditRestores = restores; |
There was a problem hiding this comment.
🟡 Race between partial-hide and the safety net: window.__contrastAuditRestores = restores is set only at the END of the walker loop. If prepare() throws mid-walk (e.g. a foreign iframe element yields an unexpected getComputedStyle, or a rare setProperty failure on a shadow-DOM node), some elements are already color: transparent !important but window.__contrastAuditRestores is still undefined — so __contrastAuditRestoreIfPending in validate.ts's finally returns early (if (!restores) return;) and the partial-hide leaks for the rest of the puppeteer page lifetime. The blast is bounded by the CLI invocation (page closes at exit), but it's a one-line fix: hoist window.__contrastAuditRestores = restores; to BEFORE the while ((node = walker.nextNode())) loop. restores is passed by reference, so subsequent restores.push(...) calls remain visible via the window property as the walk progresses. Same fix applies to contrast-report.mjs. — Rames D Jusso
| // inline value (or remove the property entirely) afterward. | ||
| var origColor = el.style.getPropertyValue("color"); | ||
| var origColorPriority = el.style.getPropertyPriority("color"); | ||
| el.style.setProperty("color", "transparent", "important"); |
There was a problem hiding this comment.
🟡 CSS transition contamination on the hidden-text screenshot: if any candidate element has a transition on color (or fill, for SVG text) in the composition's stylesheet, the setProperty("color", "transparent", "important") here starts a transition rather than an instantaneous switch. The screenshot fires ~immediately after prepare() returns via page.evaluate, but there's a small window (single-digit ms) where glyphs are mid-transition and still partially visible — which corrupts the "hidden text" premise the whole two-phase approach rests on, since the sampled pixels would include a fading glyph tint rather than the true composited background. Real-world compositions with animated text colors are rare, but this is the load-bearing assumption of the fix. Cheap defensive addition: also set transition: none !important on color (and fill when isSvgText) in prepare, capture their prior values in the same restores entry, and restore in __contrastAuditRestoreAll. — Rames D Jusso
| // partially-overlapping translucent decoration in ways a ring isn't. | ||
| const candidates = (await page.evaluate(() => | ||
| typeof (window as unknown as Record<string, unknown>).__contrastAuditPrepare === "function" | ||
| ? ((window as unknown as Record<string, unknown>).__contrastAuditPrepare as () => unknown)() |
There was a problem hiding this comment.
🟢 The prepare() page.evaluate runs OUTSIDE the try { screenshot + finish } finally { restore } scope — if it rejects with the DOM walker mid-flight (see also contrast-audit.browser.js line 230), no restore fires because the finally block hasn't started. With the browser-side fix suggested there, this becomes defense-in-depth rather than load-bearing, but broadening the try to try { const candidates = await ...prepare(...); ...screenshot; ...finish; } finally { restore } closes both gaps at once with one wrap-around. — Rames D Jusso
| bbox: { x: rect.x, y: rect.y, w: rect.width, h: rect.height }, | ||
| }); | ||
| } | ||
| window.__contrastReportRestores = restores; |
There was a problem hiding this comment.
🟡 Mirrored version of the contrast-audit.browser.js:230 concern — window.__contrastReportRestores = restores is set only at the END of the walker loop, so a mid-walk throw leaves elements hidden with no restorable state (restoreTextElements reads window.__contrastReportRestores and no-ops if it's null). Same one-line fix: hoist the assignment above the for (const node of walkTextElements(...)) loop, or equivalent placement above the walker. Applying the fix in both files at once keeps the two paths in structural sync as the header comment demands. — Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM concurring with Rames-D — two-phase capture is a real architectural improvement over the ring heuristic; concurring with Rames-D's three 🟡 holds and one 🟢 defense-in-depth ask, all non-gating.
Verified independently at head e0ff3b73.
Verified
- Two-phase contract mechanism is sound.
__contrastAuditPreparewalksdocument.body, hides each candidate's own paint viael.style.setProperty("color", "transparent", "important")(plusfillforownerSVGElement-carrying nodes), builds an ordered restore list atwindow.__contrastAuditRestores. Caller takes ONEpage.screenshot()(packages/cli/src/commands/validate.ts:337), passes base64 into__contrastAuditFinish, which — critically — calls__contrastAuditRestoreAll()BEFORE decoding the image (packages/cli/src/commands/contrast-audit.browser.js:230), so a slow/failedImagedecode can never leave the page stuck with invisible text. - Restore is layout-neutral. Only
colorandfillare touched, and only via inlinestyle(no class swaps, no attribute writes tofill=""); prior inline value + priority are preserved exactly (contrast-audit.browser.js:181-192). No reflow, so subsequentgetBoundingClientRectreads inside the same iteration are still valid. - Screenshot-throw + finish-throw safety net.
runContrastAuditwrapstry { screenshot + finish } finally { __contrastAuditRestoreIfPending() }(validate.ts:359-368). Ifpage.screenshot()throws, or the finishpage.evaluatethrows before its internal__contrastAuditRestoreAll()runs, the outer finally still restores — provided__contrastAuditRestoreswas populated. (The prepare-throw path is not covered — see Rames-D's holds below.) - SVG fill-vs-color fix (pattern #1).
contrast-audit.browser.js:157—tryParseSolidColor(cs.fill) || parseColor(cs.color), guarded byownerSVGElement.contrast-fg.test.tscoversnone,context-fill,url(#grad), garbage values, alpha preservation. - Cross-component bleed (pattern #2). Old ring sampled outside
bbox(the neighbor panel); new sampler iscomputeSampleRect(bbox)(contrast-audit.browser.js:263-267, unit-tested incontrast-sample.test.ts) with a 1px anti-aliasing inset, reading pixels inside the text's own bbox. The neighbor panel is structurally invisible to this sampler. - Backdrop-filter glass text (pattern #3). Same mechanism — sampling inside the bbox reads whatever the compositor produced there.
backdrop-filtercomposites into the panel's own box, so the blurred/tinted result lands in the median. - Partially-overlapping translucent decoration (pattern #4). Bounded grid
sampleGridPoints(up to 13×7 samples with astepX = max(1, (x1-x0)/12)/stepY = max(1, (y1-y0)/6)scan) covers the bbox interior; median filter — a >50% overlap moves the median, which matches the intended "partial decoration should count" semantics. - No stragglers on the old
__contrastAuditAPI. Code search returns only the browser script,validate.ts(uses new API),layout-audit.browser.test.ts(updated in this PR), and a.fallowrc.jsoncallowlist comment. - Regression tests.
contrast-sample.test.ts(5 rect + 3 grid cases),contrast-fg.test.ts(7 fg-resolution cases), plus the migrated clip-path visibility test inlayout-audit.browser.test.ts— exercises the full DOM-walk → hide → sample flow via happy-dom. - CI. All required checks pass at read time.
Render on windows-latestandTests on windows-lateststill pending — historically non-gating. - Blast radius stays inside CLI validate. Audit called only from
validateInBrowserunderif (opts.contrast). No Studio/SDK/engine consumers;frameCapture.ts's reference is a doc comment about thepage.addScriptTaginjection pattern, not a real dep. - Commit envelope is clean. No
Co-Authored-By: Claudein either commit; no🤖 Generated with [Claude Code]in the PR body.
Concurring with Rames-D's holds
All three 🟡 and the 🟢 ask land; the first two hit the load-bearing assumption of the two-phase design.
- 🟡
contrast-audit.browser.js:230— walker-loop race.window.__contrastAuditRestores = restoresis set at the END of the walker, so a mid-walkprepare()throw leaves elements already hidden withrestoresnever assigned towindow. The outer finally's__contrastAuditRestoreIfPendingreadsif (!restores) return;and no-ops. Concur on the one-line fix: hoist the assignment ABOVE the walker loop —restoresis the same array reference either way. - 🟡
contrast-audit.browser.js:202—transition: colorcontamination. This is the load-bearing hazard for the whole two-phase premise. If any candidate hastransition: color <t>in a stylesheet,setProperty("color", "transparent", "important")starts an animation rather than an instantaneous switch, and the screenshot fires within a single-digit-ms window during which glyphs are still partially visible. Real-world compositions with animated text colors are uncommon but not zero, and the failure mode is silent (contrast ratios computed against a slightly tinted background). Concur — Rames-D's ephemeraltransition: none !importantoverride (or agetComputedStyleskip on nonzerotransitionDurationforcolor/fill) is the least invasive countermeasure. - 🟢
validate.ts:314—prepare()outside try/finally. Defense-in-depth once the browser-side hoist above lands; broadening the outer try to wrapprepare()too closes the gap at both layers with one edit. - 🟡
contrast-report.mjs:223— mirrored version of the walker-loop race. Same END-assignment shape (window.__contrastReportRestores = restores). The header comment demands structural sync between the two paths — fix both at once.
Observations (non-blocking, unique to this review)
- O1 — shadow DOM / iframe blind spot.
document.createTreeWalker(document.body, NodeFilter.SHOW_ELEMENT)doesn't cross shadow roots or same-origin iframes. Pre-existing (old code used the same walker) but the "sample real pixels" framing invites the question — if a shadow-DOM<hyperframes-caption>-style element ever ships, the audit will silently skip it rather than crash. Latent, not a regression. - O2 — CSS
content: attr(...)and::before/::afterglyphs. Elements whose only visible text is a pseudo-element fail the direct-text-nodehasTextcheck (contrast-audit.browser.js:118) and are filtered. Pre-existing. Pseudo-element text on an element that also has direct text does hide via inheritance from the inlinecolor— that half works. - O3 — inline
!importanthide vs author-side!important. Contrived, but a stylesheet rule of the form[style*="color"] { color: red !important }could beat the inline hide on specificity tie-breaks. Not a real composition pattern today; naming for completeness. - O4 — GPU vs software compositor variance. Empirical before/after ratios in the PR body are single-config. Median + 1px inset should absorb sub-pixel AA differences, but
PRODUCER_BROWSER_GPU_MODE=software(validate.ts:459-463) exercises a different rasterizer path — worth spot-checking once if convenient.
R1 by Via
…es and slow transitions Four follow-ups from review of the hide-text/sample-bbox background change: - Register the restores list (window.__contrastAuditRestores / __contrastReportRestores) BEFORE the DOM walk starts, and push into it incrementally as each element is hidden, instead of assigning it only after the walk finishes. If getComputedStyle/ getBoundingClientRect throws partway through (a detached or otherwise pathological element), everything hidden before the throw is now still reachable for restore instead of leaking hidden until the next full audit. Repro'd directly: monkey-patched the second of three text elements' getBoundingClientRect to throw mid-walk — before this fix the first element's hide was unrecoverable since the restores array was never published; after, __contrastAuditRestoreIfPending() correctly restores it. - Force `transition: none` (important) alongside color/fill when hiding text, and restore it alongside them. A `transition` on color/fill otherwise animates the hide instead of applying it instantly, so the screenshot taken right after can land mid-transition and catch a partially-visible glyph, contaminating the background sample. Repro'd with a `transition: color 4s` element: before this fix, direct pixel sampling of the "hidden" screenshot showed a fully unhidden (pure original-color) pixel at the glyph edge on every run; after, zero contamination across repeated runs. - Move the __contrastAuditPrepare() call inside the try block in validate.ts's runContrastAudit, as its first statement, so a throw from prepare() itself is covered by the existing finally-restore-safety-net. Previously prepare() ran before the try, so a mid-prepare failure skipped the restore entirely. - Added a regression test locking in the "solid-fill pill is already correct" investigation finding: constructs a dark pill/button on a bright busy page background with real (non-flat) per-pixel screenshot data, exercises the actual two-phase prepare()/finish() path, and asserts the text isn't flagged — so a future change to the bbox-sampling logic that regressed this case would fail loudly instead of silently. All four changes mirrored between contrast-audit.browser.js and skills/hyperframes-creative/scripts/contrast-report.mjs where applicable (the report script doesn't have a separate caller-side try/finally to fix — its own try/finally around the screenshot+measure call already wraps prepare()).
3 participants
What
Fixes five reported false-positive/false-negative patterns in the WCAG contrast audit (
hyperframes validate --contrast):colorinstead of SVGfill.Why
The audit estimated an element's background two ways:
getComputedStyle(el).color— wrong for SVG<text>/<tspan>, which is painted viafill, an independent CSS property.background-colorfor solid pills/buttons.The ring is a proximity heuristic. It's wrong whenever what's immediately outside the text differs from what's actually behind it:
backdrop-filter: blur()glass panel sized only a couple pixels larger than the text — the ring exits the panel into the raw, unblurred, untinted backdrop.How
SVG fill (#1): elements inside an
<svg>(el.ownerSVGElement) now prefer the computedfillwhen it resolves to a solidrgb()/rgba()color, falling back tocolorfor paint values that aren't a plain color (none,context-fill, gradient/pattern refs).Cross-comp bleed / glass blur / partial decoration (#2–#4): replaced the ring-sampling + own-background-ancestor-walk heuristic with a two-phase capture:
__contrastAuditPrepare()walks the DOM, computes each candidate's foreground (unchanged logic from Initial repo setup #1), and hides that element's own text paint (color/fill→transparent, layout-neutral — no reflow).__contrastAuditFinish(imgBase64, time, candidates)restores the original paint immediately, then samples the real composited pixels directly inside each element's own bbox — no proximity heuristic needed, since these are the exact pixels that were behind the glyphs.This is a real architectural change to
contrast-audit.browser.js's calling contract (single__contrastAudit→__contrastAuditPrepare/__contrastAuditFinish), withvalidate.ts'srunContrastAuditupdated to match, including a try/finally restore-safety-net so a mid-loop screenshot/decode failure can't leave a later sample auditing a page with stale hidden text.Mirrored the identical change in
skills/hyperframes-creative/scripts/contrast-report.mjs, which duplicates the same DOM-walk/sampling logic (not just the WCAG math). There, the visible frame for the human-facing overlay image still comes from the producer's normalcaptureFrameToBufferpath (unchanged); only the background-sampling capture is a plainsession.page.screenshot()taken after hiding text — deliberately bypassingcaptureFrameToBuffer, whose static-frame dedup cache knows nothing about the DOM mutation and would hand back a stale pre-mutation buffer.Solid-fill pill (#5): reproduced a rounded pill/button with a busy page background outside it. The existing own-background ancestor walk already resolves the pill's declared
background-colorcorrectly regardless of the rounded corners — confirmed via repro, both before and after this change report the identical (correct) result. No fix needed; left untouched, and this case is covered by the new architecture too (would give the same right answer even without the ancestor-walk fallback).Added
packages/cli/src/commands/contrast-sample.ts(mirroring the existingcontrast-bg.ts/contrast-fg.tspattern) hosting the pure sample-rect/grid-point computation, unit tested — the browser-injected scripts can't import it directly, so it's kept in sync by hand, same convention as the rest of this file.Test plan
contrast-fg.test.ts(SVG fill resolution),contrast-sample.test.ts(sample-rect clamping/degenerate cases), plus the fullpackages/clisuite (1424 tests) passes, including an updatedlayout-audit.browser.test.tscase that called the old single-function__contrastAuditAPI directly.puppeteer-coreharness against realchrome-headless-shell, one minimal HTML fixture per pattern, comparing the audit's reported ratio/verdict against a hand-constructed ground truth:fill:white/ nocoloron black bg → before:fg=rgb(0,0,0)ratio1:1(false FAIL); after:fg=rgb(255,255,255)ratio21:1(correct PASS).bg=rgb(255,255,255)ratio1.23:1(false FAIL); after:bg=rgb(0,0,0)ratio17.14:1(correct PASS).backdrop-filter: blur(14px)panel over a yellow/blue gradient, panel only ~2px larger than the text → before:bg=rgb(0,64,255)(raw gradient color, blur/tint completely missed) ratio3.18:1(false FAIL); after:bg=rgb(159,160,165)(correct blurred/tinted blend) ratio8.05:1(correct PASS).bg=rgb(16,16,16)(ring never touches the badge, which sits entirely inside the bbox) ratio17.45:1(false PASS); after:bg=rgb(171,171,171)(correctly detects the badge) ratio2.11:1(correct FAIL).bg=rgb(10,10,10)ratio19.8:1before and after.hyperframes validate --contrastCLI command (viatsx src/cli.ts) against a real scaffolded project containing all 4 patterns simultaneously — only the genuinely-failing case (the 92%-covered decoration) is reported (1.09:1, need3:1); the cross-comp-bleed, glass-blur, and solid-pill cases are correctly silent. A second vanilla scaffold with plain white-on-dark text produces zero false positives.oxlint,oxfmt --check, andtsc --noEmitall pass on the changed files.